New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Sell > Catalog > Catalog price rule create/edit action #13716
Migrate Sell > Catalog > Catalog price rule create/edit action #13716
Conversation
@@ -58,8 +58,11 @@ public function getData($catalogPriceRuleId) | |||
$editableCatalogPriceRule = $this->bus->handle(new GetCatalogPriceRuleForEditing((int) $catalogPriceRuleId)); | |||
|
|||
$price = $editableCatalogPriceRule->getPrice(); | |||
$leaveInitialPrice = false; | |||
|
|||
if (-1.0 === $price) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch out with float comparaison in PHP.
*/ | ||
_handle() { | ||
const checkboxVal = $(`${this.$checkboxSelector}:checked`).val(); | ||
const isFieldEnabled = parseInt(checkboxVal, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's 10
?
$specificPriceRule->from_quantity = $command->getFromQuantity(); | ||
$specificPriceRule->price = $command->getPrice(); | ||
$specificPriceRule->from = $command->getFrom(); | ||
$specificPriceRule->to = $command->getTo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that getFrom()
and getTo()
should have better naming. Maybe getDateFrom()
or similar, wdyt?
$specificPriceRule->to = $command->getTo(); | ||
$specificPriceRule->reduction_type = $command->getReductionType(); | ||
$specificPriceRule->reduction_tax = $command->isTaxIncluded(); | ||
$specificPriceRule->reduction = $command->getReduction(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe getReductionAmount()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, doesn't seem that it would clarify much, also because of reduction types being amount
and percentage
$groupId, | ||
$fromQuantity, | ||
$reduction, | ||
$shopId = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is shop association really optional?
/** | ||
* Provides data transfer object for editing CatalogPriceRule | ||
*/ | ||
class GetCatalogPriceRuleForEditing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed before, should we rename it to GetCatalogPriceRuleForEditForm
? ping @matks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed before, should we rename it to
GetCatalogPriceRuleForEditForm
? ping @matks
I guess you mean rename to 'GetCatalogPriceRuleForUpdating' ? or smth associated with update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean GetCatalogPriceRuleForEditForm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I might have forgotten something here :/ why do you want to modify the naming ?
I'm not against it, I just want to understand 😅
And then we might need to write a convention and modify it everywhere
/** | ||
* Provides data for editing CatalogPriceRule | ||
*/ | ||
class EditableCatalogPriceRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be called CatalogPriceRuleForEditForm
. ping @matks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see all the other handlers are named like Get[name]ForEditing
/** | ||
* @param CommandBusInterface $commandBus | ||
* @param $isMultishopEnabled | ||
* @param $contextShopId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type hints.
@matks, ready for review |
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function handle(AddCatalogPriceRuleCommand $command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matks, I found this smartyCache cleaning unnecessary, but I'm not really sure, wdyt?
Tools::clearSmartyCache(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can read about that here : #13627 (comment)
In fact cache need to be cleared in order to update price in all existing smarty cache but doing here is not really good as you can read on my report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting, thanks @Matt75 !
@zuk3975 I suggest we remove it for now but create a dedicated issue for that.
I can imagine 3 solutions for this issue (there might be more):
- display a warning message after a catalog price rule has been created "eyh ! you created a rule, you probably want to clear the cache to make sure FO display is right !" with a link/button to perform the clear (additionnaly: using an ajax request so we dont freeze the screen ❤️)
- put a timer to clear cache 1 minute after last catalog price rule modification in order to avoid to perform 1 clear by rule created
- allow merchant to control whether he wants to clear the cache manually or automatically with a boolean configuration value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a reminder issue for that #14761
src/Core/Domain/CatalogPriceRule/Command/AddCatalogPriceRuleCommand.php
Outdated
Show resolved
Hide resolved
}) }} | ||
|
||
{{ ps.form_group_row(catalogPriceRuleForm.reduction_type, {}, { | ||
'label': 'Reduction Type'|trans({}, 'Admin.Catalog.Feature'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, please write Reduction type without the second capital letter, thank you.
src/PrestaShopBundle/Resources/views/Admin/Sell/Catalog/CatalogPriceRule/Blocks/form.html.twig
Show resolved
Hide resolved
; | ||
|
||
if ($this->isMultishopEnabled) { | ||
$builder->add('id_shop', ChoiceType::class, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matks, I see that shop field is not in the specs doc. Is it missed by mistake (I guess, yes, otherwise we don't know which shop to target), or it really shouldn't be there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @marionf who will have the answer 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @marionf again 😄
The specifications doc does mention the "shop" input for multistore, is that expected or a small mistake ?
use Symfony\Component\Validator\Constraint; | ||
|
||
/** | ||
* Provides reduction validation by reduction type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraint for validating reduction
?
$message = $constraint->invalidPercentageMessage; | ||
} | ||
|
||
if (DomainConstraintException::INVALID_REDUCTION_TYPE === $exceptionCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could merge it into elseif
.
|
||
public $invalidAmountMessage = 'Reduction value is invalid.'; | ||
|
||
public $invalidPercentageMessage = 'Reduction value is invalid.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need 3 different properties for messages? Aren't 2 enough:
$invalidTypeMessage
- when reduction type is invalid (for example type is not percent
nor amount
)
$invalidValueMessage
- when reduction value is invalid (for example value is 110 when percent is type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the cap of 100% should be applied only for percentage value, else its constrained to be >= 0. So the error messages differs a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the default messages in constraints, now it should make sense
$fromQuantity, | ||
$reductionType, | ||
$reductionValue, | ||
$shopId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it only be created for single shop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to legacy - yes, according to current specs - i don't see shop selection, but waiting for @matks response if it's not missed by mistake.
/** | ||
* Provides data transfer object for editing CatalogPriceRule | ||
*/ | ||
class GetCatalogPriceRuleForEditing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean GetCatalogPriceRuleForEditForm
.
$to, | ||
$reductionType, | ||
$includeTax, | ||
$reduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think Reduction
VO could be used instead?
* @param float $reduction | ||
*/ | ||
public function __construct( | ||
$catalogPriceRuleId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use VO here.
{ | ||
return [ | ||
$this->translator->trans('Amount', [], 'Admin.Global') => 'amount', | ||
$this->translator->trans('Percentage', [], 'Admin.Global') => 'percentage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using constants for amount
and percentage
from VO?
]) | ||
->add('reduction', ReductionType::class, [ | ||
'constraints' => [ | ||
new ReductionConstraint($this->getReductionMessages()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Constraint
suffix is not needed.
* <input type="text" class="form-control" | ||
* data-format="YYYY-MM-DD HH:mm:ss" // provide data-format attr in case you need custom format | ||
* /> | ||
* </div> | ||
*/ | ||
const init = function initDatePickers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a more accurate name for this class, because we might need multiple date-pickers extensions. Some with YYYY-MM-DD, some with only YYYY-MM, some side-by-side, some other displays ... so we need to name them accurately in order to avoid confusion and pick the one we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I would like to wait for #14748 this to be solved, as I guess it will modify this js file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const $ = window.$; | ||
|
||
$(() => { | ||
new PriceFieldAvailabilityHandler('#catalog_price_rule_leave_initial_price', '#catalog_price_rule_price'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a dedicated JS file to contain these selectors, like here:
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/new-theme/js/pages/employee/employee-form-map.js
And use it like this
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/new-theme/js/pages/employee/EmployeeForm.js
This will help our friends from the QA team to target them and build E2E tests 😉
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function handle(AddCatalogPriceRuleCommand $command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting, thanks @Matt75 !
@zuk3975 I suggest we remove it for now but create a dedicated issue for that.
I can imagine 3 solutions for this issue (there might be more):
- display a warning message after a catalog price rule has been created "eyh ! you created a rule, you probably want to clear the cache to make sure FO display is right !" with a link/button to perform the clear (additionnaly: using an ajax request so we dont freeze the screen ❤️)
- put a timer to clear cache 1 minute after last catalog price rule modification in order to avoid to perform 1 clear by rule created
- allow merchant to control whether he wants to clear the cache manually or automatically with a boolean configuration value
* | ||
* @throws PrestaShopException | ||
*/ | ||
private function createSpecificPriceRuleFromCommand(EditCatalogPriceRuleCommand $command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not exactly create
right ? 😄
Fetch ? Get ? Populate ? Hydrate ?
sprintf('Failed to create specific price rule') | ||
); | ||
} | ||
$specificPriceRule->deleteConditions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice operations split
*/ | ||
private function createSpecificPriceRuleFromCommand(AddCatalogPriceRuleCommand $command) | ||
{ | ||
$specificPriceRule = new SpecificPriceRule(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be boring to write these fields, but it's definitely very readable and maintanable 👍
sprintf('Failed to update specific price rule with id %s', $specificPriceRule->id) | ||
); | ||
} | ||
$specificPriceRule->deleteConditions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to call this everytime we edit it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least its how its done in legacy, but this should be checked more in-depth on separate PR of condition groups. Added a reminder here #10549 (comment)
$specificPriceRule->reduction_tax = $command->isTaxIncluded(); | ||
} | ||
if (null !== $command->getReduction()) { | ||
$specificPriceRule->reduction_type = $command->getReduction()->getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting 🤔
private $fromQuantity; | ||
|
||
/** | ||
* @var Reduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea 👍
} | ||
|
||
$this->context->buildViolation($message, $params) | ||
->setTranslationDomain('Admin.Notifications.Error') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check it works (check with 2 languages) ? I believe @mickaelandrieu told me once we could not use Symfony validator because there was only 1 translation catalog available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is not working 😞
} | ||
|
||
try { | ||
new Reduction($value['type'], $value['value']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're using validation checks in Reduction
that throw DomainConstraintExceptions
and catch them in order to write the right error message and avoid breaking the flow ..?
Euh ... 🤔it works but it hijacks the standard workflow of these validation checks.
I've never seen this way of validating an object 😅 it might feel weird, but maybe it's right.
Need more opinion, ping @PierreRambaud @jolelievre @eternoendless @mickaelandrieu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look weird to use a constructor to validate data 🤔
ping @eternoendless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @PierreRambaud, please move the validation logic outside of Reduction constructor 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matks although I agree that it's not the most convenient way to validate data, but it works.
We could of course duplicate validation that is inside Reduction
constructor, but we can't move it out. Do you have anything else on your mind? :)
/** | ||
* @return Reduction | ||
*/ | ||
public function getReduction(): Reduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
php7 💪
I ❤️ that !
{ | ||
try { | ||
return new DateTime($dateTime); | ||
} catch (Exception $e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure every Exception thrown from previous line are because of bad format 😄?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the phpdoc says, yes.
If time contains an invalid date/time format, then an exception is now thrown. Previously an error was emitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, exception should be thrown only due to wrong format 😲 . Imo, the current message looks clear anyway
09e1b75
to
0da87c9
Compare
/** | ||
* @var float|null | ||
*/ | ||
private $price; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should start using Number
from https://github.com/PrestaShop/decimal here to carry the data
Sure, it'll get casted to float when the data is thrown back in the legacy
But at least it'll be the right data structure in the migrated part of the code
Wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | ||
* Provides data transfer object for editing CatalogPriceRule | ||
*/ | ||
class GetCatalogPriceRuleForEditing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I might have forgotten something here :/ why do you want to modify the naming ?
I'm not against it, I just want to understand 😅
And then we might need to write a convention and modify it everywhere
/** | ||
* Thrown when unable to update catalog price rule | ||
*/ | ||
class UpdateCatalogPriceRuleException extends CatalogPriceRuleException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CannotUpdateCatalogPriceRuleException
? FailureToUpdateCatalogPriceRuleException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the exception
suffix should already indicate that something failed
/** | ||
* Provides data for editing CatalogPriceRule | ||
*/ | ||
class EditableCatalogPriceRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
*/ | ||
private function assertIsAllowedType(string $type) | ||
{ | ||
if ($type !== self::TYPE_AMOUNT && $type !== self::TYPE_PERCENTAGE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use an in_array()
, it will be easier to add new types later 😉
; | ||
|
||
if ($this->isMultishopEnabled) { | ||
$builder->add('id_shop', ChoiceType::class, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @marionf who will have the answer 😄
path: / | ||
methods: [GET] | ||
defaults: | ||
_controller: 'PrestaShopBundle:Admin\Sell\Catalog\CatalogPriceRule:create' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not create
I think 😄
b0926c9
to
1b300df
Compare
Hello, FYI i've just created a new issue to change some wording on this page. |
/admin-dev/index.php/sell/catalog/catalog-price-rules
link to reach migrated catalog price rules list. Click edit or create new to see the form. The Condition groups (at the bottom of the page) are not migrated yet and should be done in separate PR.This change is